Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NO MERGE] PoC Dagger DI as a system builder #8411

Draft
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

Nashatyrev
Copy link
Contributor

@Nashatyrev Nashatyrev commented Jun 27, 2024

PR Description

This is PoC on how the Google Dagger compile time Dependency Injection library could be used to build up the Teku components together.

This PR demonstrates the concept by building BeaconChainController in a modular fashion with clear dependencies for every small subcomponent.

Pros of Dagger approach:

  • Less builder boilerplate code
    • Easier to add new components
    • Easier to add new dependencies for the components on lower levels: no need to pass a new parameter down through the whole chain of builders
    • The larger component (e.g. Eth2Network) is assembled from smaller subcomponents (DiscoveryNetwork, Eth2PeerManager, etc.) but there is no need for a larger component to care about passing parameters to its subcomponents
  • Compile-time dependencies resolving
    • It looks more reliable and less error-prone than the present BeaconChainController initialization
    • Dagger compiler is pretty strict, it errors on any missing/duplicate/circular dependency.
    • We may also create and add an SPI plugin to check unused dependencies for production
    • No runtime reflection (different than the majority of DI libs): all the glue source code is generated by Dagger during compilation
  • Modularity:
    • Easy to acceess/replace/wrap any minor component of Teku on any level
    • Could make Teku the best platform for experiments and protyping
  • Dagger generated code is used during Teku construction only (with a minor exception of Lazy dependencies)
  • IDEA standard plugin is pretty convenient to navigate the dependency graph

Cons:

  • Another system which developers need to be familiar with
  • Less implicit code
    • No more ability to navigate call hierarchy in the builders code (there is the Dagger plugin mentioned above though)
  • No incapsulation: with the current design all dependencies are visible to all components on all levels.
  • Potentially slower building (not sure what is the exact slowdown)

As an experiment I have also created a draft Dagger builder for the networking components in this branch

TODOs:

  • Add testing of the BeaconChainController
  • Expand the approach to other subsystem building (like Network, ValidatorClient, Storage)
  • Detect unused dependencies
  • Dagger doesn't allow to easily replace just one dependency in the module, the whole module needs to be replaced. Probably it makes sense to implements another annotation processor to address this if needed.
  • There are few components that are initialized asynchronously. Currently it's addressed by simply calling SafeFuture.join(). Dagger has the extension 'Producers' which deals asynchronously with all the dependecies. Worth trying to adopt it.
  • There is a number of 'Void' dependencies which initialize something but don't return any specific component. They are now collected to a Set<VoidInitializer> dependecy for a top level ServiceStarter component. Probably there is a better approach.
  • For various classes which need to be subscribed to specific channels I would better explicitly pass the corresponding EventChannelSubscribers to their constructors/builders
  • Dagger Lazy is used in 2 distinct contexts:
    • to break the loops in dependencies
    • to conditionally choose one child component during composition depending on the config and skip initialting of others
      Lazy doesn't perform loop checking during compile time, however in the second case it makes sense and worth to be done
  • Add explicit BLS dependency instead of static instance
  • Find out the safest way to adopt the changes to production code

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

…ata initialization so we need to add this dummy dependency
@lucassaldanha lucassaldanha added the DO NOT MERGE Not ready to merge label Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Not ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants